Automated Test: sms-retry-enhanced #364
Conversation
* add retry count to workflow reminder * add logic to for retry count --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com> Co-authored-by: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com>
📝 WalkthroughWalkthroughThis pull request introduces retry count tracking to SMS reminder scheduling. It adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts`:
- Around line 29-45: The deleteMany OR clause currently removes any
WorkflowReminder with retryCount > 1 regardless of method; update the
prisma.workflowReminder.deleteMany call so the retryCount condition is scoped to
SMS — either add method: WorkflowMethods.SMS to the retryCount branch or
refactor the where to require method: WorkflowMethods.SMS for both conditions
(referencing prisma.workflowReminder.deleteMany and WorkflowMethods.SMS and the
retryCount field).
🧹 Nitpick comments (1)
packages/features/ee/workflows/api/scheduleSMSReminders.ts (1)
189-198: DB update insidecatchcan throw unhandled, halting the loop for remaining reminders.If
prisma.workflowReminder.updateat line 190 throws (e.g., DB connectivity issue, or the reminder was already deleted by thedeleteManyabove in a race), the exception propagates out of theforloop and the remaining unscheduled reminders won't be processed in this cron run.Wrap the update in its own try-catch to keep the loop going:
♻️ Proposed fix
} catch (error) { + try { await prisma.workflowReminder.update({ where: { id: reminder.id, }, data: { retryCount: reminder.retryCount + 1, }, }); + } catch (updateError) { + console.log(`Error updating retryCount for reminder ${reminder.id}: ${updateError}`); + } console.log(`Error scheduling SMS with error ${error}`); }
| await prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| OR: [ | ||
| { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
retryCount > 1 branch is not scoped to SMS — will delete EMAIL/WHATSAPP reminders too.
The first branch correctly filters by method: WorkflowMethods.SMS, but the second OR branch matches any WorkflowReminder with retryCount > 1 regardless of method. This will inadvertently delete email and WhatsApp reminders that happen to have a non-zero retry count (if retry logic is ever added for those methods, or if retryCount is manually set).
🐛 Proposed fix: scope retryCount condition to SMS
await prisma.workflowReminder.deleteMany({
where: {
- OR: [
- {
- method: WorkflowMethods.SMS,
- scheduledDate: {
- lte: dayjs().toISOString(),
- },
- },
- {
- retryCount: {
- gt: 1,
- },
- },
- ],
+ method: WorkflowMethods.SMS,
+ OR: [
+ {
+ scheduledDate: {
+ lte: dayjs().toISOString(),
+ },
+ },
+ {
+ retryCount: {
+ gt: 1,
+ },
+ },
+ ],
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await prisma.workflowReminder.deleteMany({ | |
| where: { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| OR: [ | |
| { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], | |
| }, | |
| }); | |
| await prisma.workflowReminder.deleteMany({ | |
| where: { | |
| method: WorkflowMethods.SMS, | |
| OR: [ | |
| { | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 29 -
45, The deleteMany OR clause currently removes any WorkflowReminder with
retryCount > 1 regardless of method; update the
prisma.workflowReminder.deleteMany call so the retryCount condition is scoped to
SMS — either add method: WorkflowMethods.SMS to the retryCount branch or
refactor the where to require method: WorkflowMethods.SMS for both conditions
(referencing prisma.workflowReminder.deleteMany and WorkflowMethods.SMS and the
retryCount field).
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Chores